feat: doctor and adopt - #17

Verdict: changes requested

Independent review, round 1. task check run against the pushed head: exit 0, every lint target
clean, eleven packages green with -race, gitleaks no leaks. Every probe below ran against a
throwaway HOME under $TMPDIR with GIT_CONFIG_NOSYSTEM=1; the real global git config was not
touched.

What holds up. A fully healthy tree exits 0 with no findings, so no check cries wolf. A
deliberate gitcd.profile pin naming an existing profile is not reported as drift, and a pin
naming a profile the configuration no longer carries is reported as stale-pin. Both global
identity keys, the ordering finding, and both config homes are reported, and --fix leaves the
global file byte for byte unchanged. Every attempt to make adopt move without consent failed
correctly: no flag with a closed stdin, an empty stdin, no, a bare newline, maybe, and
--dry-run with yes on stdin all refused and left the source in place. Every occupied
destination (non-repository directory, empty directory, a repository with a different remote, a
repository with the same remote) was refused before any move, with the source intact. A read only
destination parent and a read only grandparent both failed cleanly with the source intact. A read
only managed directory and an unreadable config file both fail loudly and keep the finding
outstanding. All nine Kind values have a fixture. I reverted three guarded behaviours in a
detached source only copy: the case rename, the consent gate and the target conflict check each
made their test fail, including the two the description names as previously non-discriminating.

Blocker

cmd/gitcd/adopt.go:66, cmd/gitcd/adopt.go:142 — adopt moves a linked worktree, and a main
checkout that has linked worktrees, and breaks git in both directions without a word.

git.IsRepository accepts a .git file, so a linked worktree passes the guard at line 66 and
os.Rename at line 142 moves it. Git's administrative link is left pointing at the old path.

Probe, adopting a linked worktree:

move .../wt-feat to .../Code/github.com/acme/gadget? [y/N] ... EXIT=0
$ git -C main-checkout worktree list
.../main-checkout  1fb1f4e [main]
.../wt-feat        1fb1f4e [feat] prunable
$ git -C main-checkout worktree prune
$ git -C Code/github.com/acme/gadget status
fatal: not a git repository: (null)

A routine git worktree prune, or git gc, then destroys the link and the adopted directory
stops being a repository. Anything uncommitted in it is orphaned.

The other direction is immediate, with no prune needed. Adopting a main checkout that has a
linked worktree elsewhere:

... EXIT=0
$ git -C wt-feat status
fatal: not a git repository: (null)

adopt printed its success report and exited 0 in both cases.

Fix: refuse a linked worktree before the move, by comparing
git rev-parse --absolute-git-dir with git rev-parse --git-common-dir, the same shape the
extensions.worktreeConfig refusal at line 71 already has; and after moving a main checkout, run
git worktree repair from the new path, or refuse when git worktree list reports more than one
entry. Either way, name it in the message rather than exiting 0.

cmd/gitcd/doctor.go:245 — doctor --fix never converges when two repositories disagree about
the case of a shared ancestor, and it un-places a correctly placed repository.

renameCaseOnly renames the topmost differing ancestor, carrying every repository under it along.
Two repositories under one owner directory whose remotes spell the owner differently
(github.com/acme/widget and github.com/Acme/other) make that ancestor contested. Before the
run only widget was mis-placed; after it only other is:

--- before --- [path-case-mismatch] github.com/Acme/widget
--- fix    --- renamed .../github.com/Acme/widget to .../github.com/acme/widget
--- after  --- [path-case-mismatch] github.com/acme/other     <- new, created by --fix

And it flips forever, moving every repository under the ancestor on each run while reporting
success:

fix run 1  renamed .../acme/other  to .../Acme/other
fix run 2  renamed .../Acme/widget to .../acme/widget
fix run 3  renamed .../acme/other  to .../Acme/other
fix run 4  renamed .../Acme/widget to .../acme/widget

Fix: before renaming an ancestor, check that every repository under it agrees on the new spelling.
Where they do not, report a non-fixable finding naming both repositories instead of renaming, so
--fix is idempotent and never moves a repository that was already correct.

Should

cmd/gitcd/doctor.go:96, cmd/gitcd/doctor.go:185, cmd/gitcd/doctor.go:206 — --json --fix
is not JSON. applyFixes writes its progress lines to stdout before the encoder runs, so the
machine readable surface is corrupted:

$ gitcd doctor --json --fix 2>/dev/null | jq -r .configPath
jq: parse error: Invalid numeric literal at line 2, column 0
$ gitcd doctor --json --fix --dry-run 2>/dev/null | jq .
jq: parse error: Invalid numeric literal at line 1, column 6

Three bare managed file paths precede the object; --dry-run prepends would rerender .... Fix:
pass stderr to applyFixes for its progress lines when --json is set, or hold those lines and
emit them only on the text path. writeRenderedFiles's own path echo needs the same treatment.

cmd/gitcd/adopt.go:138 — a target under the source leaves new directories inside the user's
repository. MkdirAll runs before the rename and is not undone when the rename fails:

$ gitcd adopt ~/Code          # layout renders ~/Code/github.com/acme/gadget
... rename ...: invalid argument      EXIT=1
$ find ~/Code -maxdepth 3
~/Code/.git
~/Code/github.com
~/Code/github.com/acme        <- created by the failed adopt, never mentioned

Nothing is lost, but the run leaves directories the user did not have and does not say so. Fix:
refuse when newPath is under oldPath before line 138, and remove what MkdirAll created when
the rename fails.

cmd/gitcd/doctor.go:206 — the rename message names the wrong thing. It prints
renamed <f.Path> to <f.WantPath>, the repository's own paths, while renameCaseOnly actually
renamed an ancestor directory that may carry many other repositories. In the probe above, run 1
printed renamed .../acme/other to .../Acme/other for a move of the whole acme directory. Fix:
return the ancestor pair renameCaseOnly acted on and print that.

cmd/gitcd/adopt.go:158 — any os.Stat error is read as "no conflict". A permission denied or
a symlink loop on newPath returns "" and the move is attempted. Fix: branch on
errors.Is(err, os.ErrNotExist) for the clear case and report every other error as a conflict.

Nit

• internal/doctor/doctor.go:321: movedProfileTarget is always defaultMovedProfileName
  ("global"), so the finding names a profile that need not exist, and still says "global" for a
  user who ran gitcd init --move-identity --profile work. Naming the profile the rules or the
  existing configuration actually imply would read truer.
• cmd/gitcd/adopt.go:158: on a case insensitive filesystem a case only adopt is refused as
  already exists with the same remote: it looks already adopted, which is not what happened.
  doctor --fix is the right path for that case; saying so in the message would save a puzzle.
• internal/doctor/doctor.go:334: global-identity-order reports one key even when several sit
  after the include. Listing them all costs nothing.
• cmd/gitcd/doctor.go:58: --json is not in issue feat: doctor and adopt #4's acceptance criteria and is the one
  surface with a defect. Worth a line in the description, or its own issue.

The Risk section's three items each check out: the root only walk is recorded in
docs/concepts/gitcd.md:85, the discover.go reuse matches AGENTS.md, and both named test
defects are genuinely fixed. Linked worktrees belong in that section too.

Ready for the implement skill to pick the rework round up.

Verdict: needs human

Round 2, the final automated round. task check run against the pushed head (b2e5441):
exit 0, every lint target clean (fmt, vet, golangci-lint 0 issues, markdownlint, yamllint,
actionlint, goreleaser check), gitleaks no leaks, eleven packages green with -race. Every
probe below ran against a throwaway HOME under $TMPDIR with GIT_CONFIG_NOSYSTEM=1; the
real global git config (/Users/simonostendorf/.gitconfig, 537 bytes) was read before and
after this review and is byte-for-byte unchanged.

Round 1 findings

• Blocker 1 (adopt moves a linked worktree / a main checkout with one): fixed. Adopting a
  linked worktree, and adopting a main checkout that still has one, both refuse with a message
  naming git worktree move/remove, and both ends stay healthy afterward. Reverting
  adoptWorktreeConflict to return "", nil in a detached, source-only copy makes both of its
  own tests fail, confirming the fix is what holds this closed.
• Blocker 2 (doctor --fix never converges on a contested ancestor): fixed for the originally
  reported two-repository case, and for three repositories disagreeing on one ancestor, and for
  a conflict nested one level below another ancestor rename that was itself uncontested — all
  converge and a second --fix run is a no-op. Reverting CheckAncestorCaseConflicts to
  return nil makes its own unit test and the black-box CLI test fail the same way round 1
  reproduced (flip-flopping, un-placing a correctly placed repository). See the new Blocker
  below: the fix that closes this introduces its own regression one layer down.
• Should 1 (--json --fix not JSON): fixed. doctor --json, --json --fix,
  --json --fix --dry-run, and a zero-finding run all produce valid JSON on stdout through
  jq .; every progress line (would rerender..., the managed-file path echo) now goes to
  stderr under --json.
• Should 2 (adopt leaves MkdirAll directories behind after a failed move): fixed for the
  two paths round 1 and this rework's own test exercise — self-nesting (refused before
  MkdirAll even runs) and a genuine os.Rename failure (the createdFrom/RemoveAll cleanup
  removes exactly what was created). See the new Should below: a third failure path, MkdirAll
  itself failing, is not covered.
• Should 3 (rename message names the repo path, not the ancestor): fixed, verified by
  TestRunDoctor_FixRenameMessageNamesTheAncestorNotTheRepoPath and by my own probe.
• Should 4 (os.Stat error read as "no conflict"): fixed, adoptTargetConflict branches on
  errors.Is(err, os.ErrNotExist) and reports every other error as a conflict.
• Nits: global-identity-order now names every key after the include, verified. The hardcoded
  "global" profile name and the misleading "already adopted" message on a case-insensitive
  filesystem are both unchanged from round 1; neither blocks this round.

Blocker

cmd/gitcd/doctor.go (filterContestedPathCase, used from collectFindings) — a genuine,
uncontested case-only path mismatch is silently dropped, never fixed and never reported, when
it happens to sit under an ancestor directory that is contested for an unrelated reason.

filterContestedPathCase drops every KindPathCase finding whose own path sits under a
contested ancestor, with no check that the finding's own mismatch is the contested segment
itself rather than a deeper, unrelated one. Reproduction: three repositories share one ancestor
directory spelled Acme; widget's remote implies acme (contested against other, which
matches Acme exactly — the reported conflict), and a third repository Mismatched also sits
under Acme, exactly matching it (no ancestor-level mismatch of its own), but its own remote
implies the leaf should read mismatched, lowercase. Both doctor and doctor --fix report
only the ancestor conflict; Mismatched's own leaf rename never appears anywhere, fixed or
outstanding. The identical fixture with the ancestor conflict removed fixes and reports the
same leaf mismatch correctly, confirming the ancestor conflict is what suppresses it.

$ gitcd doctor   # Acme/widget wants acme, Acme/other matches Acme, Acme/Mismatched wants Acme/mismatched
[ancestor-case-conflict] .../Acme: ... "Acme" (Acme/Mismatched, Acme/other), "acme" (Acme/widget) ...
# nothing else: Acme/Mismatched's own case-only rename to Acme/mismatched is never mentioned

Fix: in filterContestedPathCase, drop a KindPathCase finding only when its own
FirstDifferingSegment ancestor equals (or is an ancestor of) the contested path, not merely
when the finding's path sits anywhere under it.

Should

cmd/gitcd/adopt.go — a failing os.MkdirAll itself, not only a failing os.Rename, leaves
behind every directory it managed to create, uncleaned and unmentioned.

The createdFrom/os.RemoveAll cleanup only runs inside the os.Rename error branch. When
os.MkdirAll(mkdirTarget, ...) fails partway through creating several missing ancestors (a
remote owner segment long enough to exceed the filesystem's name limit, for instance), every
ancestor it created before failing is left on disk:

$ gitcd adopt --yes widget   # remote owner is 300 characters
gitcd adopt: create .../Code/host.com/aaaa...aaaa: mkdir .../Code/host.com/aaaa...aaaa: file name too long
$ find Code -maxdepth 2
Code/host.com   <- created by the failed adopt, never mentioned, never removed

Fix: compute createdFrom before the MkdirAll call as it already does, and remove it on a
MkdirAll failure too, not only on a Rename failure.

Nit

• cmd/gitcd/adopt.go, git.IsRepository: a bare repository (no .git entry; the directory
  itself is the git directory) is refused as "is not a git repository", which is safe but
  inaccurate — it is one, just not in the shape this check expects.
• cmd/gitcd/adopt.go, adoptWorktreeConflict: a linked worktree whose main checkout no
  longer exists is refused safely, but through a bare exit status 128 with git's own stderr
  not folded into the message, unlike every other refusal here.

Graft check

In a detached, source-only scratch copy (no .git, never pushed, removed after this review):
reverting adoptWorktreeConflict to a no-op fails both Blocker 1 tests; reverting
CheckAncestorCaseConflicts to return nothing fails its own unit test and
TestRunDoctor_FixRefusesAContestedAncestorAndConverges; reverting the self-nesting pre-check
in runAdopt fails TestRunAdopt_RefusesMovingIntoItselfAndLeavesNoDirectoryBehind (and, run by
hand, confirms the separate os.Rename-failure cleanup still removes the leftover directory on
its own, independently of the pre-check). All three reverts were restored; the scratch copy
re-builds and its tests pass clean afterward.

The two fixed blockers hold up under direct attack and under three- and four-repository,
nested-conflict and false-positive probes beyond round 1's own two-repository case. The new
Blocker is a real regression the ancestor-conflict fix itself introduces: a correct, unrelated
rename a user needs can now vanish with no trace. Round 2 is the configured limit for an
automated verdict, so this goes to a human rather than back around again.